-
Notifications
You must be signed in to change notification settings - Fork 804
Fix: Enable custom group_id in add_episode MCP tool #486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…x/mcp-group-id-handling
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 7aba34b in 1 minute and 39 seconds. Click for details.
- Reviewed
188
lines of code in3
files - Skipped
2
files when reviewing. - Skipped posting
5
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. Makefile:29
- Draft comment:
Consider adding a newline at the end of the file to adhere to standard POSIX formatting practices. - Reason this comment was not posted:
Comment was on unchanged code.
2. mcp_server/graphiti_mcp_server.py:1
- Draft comment:
This Python file is missing a copyright notice header. Please include the standard license header as required. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. mcp_server/graphiti_mcp_server.py:827
- Draft comment:
The nested ternary for assigning effective_group_ids in search_nodes is a bit hard to read. Consider refactoring this into multiple statements or adding explicit parentheses for clarity. - Reason this comment was not posted:
Confidence changes required:70%
<= threshold80%
None
4. pyproject.toml:21
- Draft comment:
The dependency specification for python-dotenv uses parentheses. Consider using the standard syntax (e.g., 'python-dotenv[cli]>=1.1.0,<2.0.0') to avoid potential issues with package resolution. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 80% While the comment suggests a valid improvement in syntax style, the parenthesized version is actually valid in pyproject.toml and Poetry. Both syntaxes work equally well. The change would be purely cosmetic and doesn't affect functionality. Package managers like pip and poetry handle both formats correctly. I might be underestimating the importance of following standard conventions. Some tools might handle the parenthesized version differently. While consistency is good, there's no evidence of actual issues with the current syntax, and both forms are valid according to PEP 508. The comment should be deleted as it suggests a purely cosmetic change without clear benefits or evidence of problems.
5. pyproject.toml:21
- Draft comment:
Typo: The dependency string 'python-dotenv[cli] (>=1.1.0,<2.0.0)' includes an extra space and parentheses compared to the conventional format. Consider changing it to 'python-dotenv[cli]>=1.1.0,<2.0.0' to follow standard dependency specification. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 80% While the suggested format is more common, both formats are valid in Poetry. The current format with parentheses is actually clearer for readability. This seems like a purely stylistic preference rather than a correctness issue. There's no strong evidence that this change is necessary or beneficial. The comment might be pointing out a consistency issue, as other dependencies in the file don't use parentheses. Also, some tools might parse these dependencies differently. The current format is valid Poetry syntax, and there's no evidence of any actual problems. The parentheses actually make the version bounds more readable. This comment should be deleted as it suggests a purely stylistic change to valid code, without providing any concrete benefit or fixing any actual issues.
Workflow ID: wflow_FLLstcPM8ZQbXGI0
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
I have read the CLA Document and I hereby sign the CLA |
@markalosey Thank you for this contribution! Just a heads up: I won't be able to review the PR for a few weeks. Expect an early June response. Apologies in advance for the delay! |
Problem:
The
add_episode
MCP tool failed when a customgroup_id
string was provided, erroring withParameter 'group_id' must be of type undefined, got string
. This prevented assigning episodes to specific, non-default groups.Solution:
This branch corrects the handling of the
group_id
parameter in theadd_episode
tool, allowing custom stringgroup_id
s to be processed successfully.Verification:
On this branch, after restarting the server:
group_id
s (e.g.,project_alpha_fix_test_001
,project_beta_fix_test_002
).search_nodes
correctly filters by a single specifiedgroup_id
.search_nodes
correctly filters by a list of specifiedgroup_id
s.This validates that adding to and searching within custom groups now works as intended.
Note on Search Behavior:
Currently,
search_nodes
without a specifiedgroup_id
defaults to the server's maingroup_id
rather than searching all groups. This could be a future enhancement if "search all" is desired for that scenario.Important
Fixes
add_episode
tool to correctly handle customgroup_id
strings and updates related functions for default handling.group_id
inadd_episode
to accept custom strings, defaulting toconfig.group_id
if empty.search_nodes
,search_facts
, andget_episodes
to handle emptygroup_id
as default.GraphitiConfig.from_cli_and_env()
now setsgroup_id
with precedence: CLI > Env Var > Default.group_id
source ininitialize_server()
.Makefile
: Updatestest
target to load.env.test
usingpython-dotenv
.pyproject.toml
: Addspython-dotenv-run
to dev dependencies.This description was created by
for 7aba34b. You can customize this summary. It will automatically update as commits are pushed.